Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: change swagger operationId output #2129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: change swagger operationId output #2129

wants to merge 1 commit into from

Conversation

compojoom
Copy link
Contributor

Summary

I'm using RTK code generation and the generated code had very ugly names. Here is an example:
grafik

I'm getting a lot of java vibes here :) Those method names are auto-generated based on the operationId.

I think that chances of collision are small if we remove the controller from the operationId.
It seems that you guys follow the convension:
AccountsController

  • createAccount
  • getAccount..
  • upsertAccount
  • deleteAccount

so despite the fact that I would remove accountsController of the name I won't have a similar operationId elsewhere as you are adding the controler name in the function

With this approach we end up with code like this:
grafik

WDYT?

@iamacook is this change going to be problematic for your client-gateway-typescript-sdk?

@compojoom compojoom requested a review from a team as a code owner November 14, 2024 17:05
@hectorgomezv
Copy link
Member

For reference, this is the complete list of operationId I get with this approach on the current codebase:

"operationId": "GetAbout",
"operationId": "v1AddConfirmation",
"operationId": "v1CheckEligibility",
"operationId": "v1CreateMessage",
"operationId": "v1CreateSubmission",
"operationId": "v1DeleteDelegate",
"operationId": "v1DeleteSafeDelegate",
"operationId": "v1DeleteTransaction",
"operationId": "v1GetAboutChain",
"operationId": "v1GetAllSafesByOwner",
"operationId": "v1GetBackbone",
"operationId": "v1GetBalances",
"operationId": "v1GetCampaignActivities",
"operationId": "v1GetCampaignById",
"operationId": "v1GetCampaignLeaderboard",
"operationId": "v1GetCampaignRank",
"operationId": "v1GetCampaigns",
"operationId": "v1GetChain",
"operationId": "v1GetChains",
"operationId": "v1GetContract",
"operationId": "v1GetCreationTransaction",
"operationId": "v1GetDataDecoded",
"operationId": "v1GetDelegates",
"operationId": "v1GetIncomingTransfers",
"operationId": "v1GetIndexingStatus",
"operationId": "v1GetLeaderboard",
"operationId": "v1GetLockingHistory",
"operationId": "v1GetLockingRank",
"operationId": "v1GetMasterCopies",
"operationId": "v1GetMessageByHash",
"operationId": "v1GetMessagesBySafe",
"operationId": "v1GetModuleTransactions",
"operationId": "v1GetMultisigTransactions",
"operationId": "v1GetNonces",
"operationId": "v1GetRelaysRemaining",
"operationId": "v1GetSafe",
"operationId": "v1GetSafeApps",
"operationId": "v1GetSafeOverview",
"operationId": "v1GetSafesByOwner",
"operationId": "v1GetSubmission",
"operationId": "v1GetSupportedFiatCodes",
"operationId": "v1GetTransactionById",
"operationId": "v1GetTransactionConfirmationView",
"operationId": "v1GetTransactionQueue",
"operationId": "v1GetTransactionsHistory",
"operationId": "v1PostDelegate",
"operationId": "v1PreviewTransaction",
"operationId": "v1ProposeTransaction",
"operationId": "v1RegisterDevice",
"operationId": "v1Relay",
"operationId": "v1UnregisterDevice",
"operationId": "v1UnregisterSafe",
"operationId": "v1UpdateMessageSignature",
"operationId": "v2GetCollectibles",
"operationId": "v2GetEstimation",

It looks good to me, but we would need to be cautious as we can't reuse method names through controllers, as the controller name would not be part of the operationId anymore @iamacook @PooyaRaki 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hectorgomezv hectorgomezv hectorgomezv approved these changes

Assignees

@compojoom compojoom

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@compojoom @hectorgomezv